The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nimbus-documentation	Ready	Preview, Comment	Apr 15, 2026 8:23am
nimbus-storybook	Ready	Preview, Comment	Apr 15, 2026 8:23am

⚠️ No Changeset found

Latest commit: 47c8ad1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude do a thorough review of this branches changes

@claude please review

Claude finished @misama-ct's task in 3m 59s —— View job

Review: refactor(card): rework Card component architecture and API

• Gather context and understand the request
• Read core implementation files
• Read slot components
• Read tests and stories
• Read docs files
• Review app-level changes
• Analyze and post review

Summary

This is a well-motivated refactor. The old context-registration pattern (useEffect → parent state via callbacks) was the worst possible approach for a compound component — it guaranteed a double-render cycle, stale-closure risk, and incompatibility with Strict Mode's double-invoke behavior. The new direct-rendering approach is idiomatic and correct. The API surface (variant/size) aligns with the rest of Nimbus, and the CSS-variable spacing model is clean. Overall the architecture is solid. A few issues need attention before merge.

Must fix before merge

1. Missing changeset

This PR contains breaking changes (Card.Content → Card.Body, four props replaced with two). The changeset bot already flagged this. Add a changeset:

pnpm changeset

Choose @commercetools/nimbus: minor (not patch — this is a breaking API change).

Should address

2. card.figma.tsx only covers 2 of 4 variants [packages/nimbus/src/components/card/card.figma.tsx:39]

The Code Connect file maps a binary "Elevated" Figma enum to elevated / outlined. The new filled and plain variants aren't represented. If the Figma component hasn't been updated yet, track this as follow-up work so the spec and implementation don't diverge silently.

3. Adjacent sibling selectors are tightly coupled to generated class names [packages/nimbus/src/components/card/card.recipe.ts:32-39]

".nimbus-card__header + &": { pt: 0 },
".nimbus-card__body + &": { pt: 0 },

These selectors depend on Chakra generating nimbus-card__header / nimbus-card__body from className: "nimbus-card" and the slot names. If either the className base or the slot names change, these selectors silently stop working. The comment explains the behavior well — add a note explaining the naming convention so the coupling is explicit:

// Class names are Chakra-generated: `${className}__${slotName}` = "nimbus-card__header"

4. Doc spec doesn't assert positive ARIA wiring [packages/nimbus/src/components/card/card.docs.spec.tsx:86-106]

The negative case (has no role or ARIA attributes without slots) correctly asserts the absence of attributes. But the positive case only checks that text renders — it doesn't verify role="article", aria-labelledby, or aria-describedby are actually set. Since this is consumer-facing documentation, it would strengthen the example:

it("renders accessible card with title and description slots", () => {
  // ...render...
  const card = screen.getByTestId("card-slot");
  expect(card).toHaveAttribute("role", "article");
  expect(card).toHaveAttribute("aria-labelledby");
  expect(card).toHaveAttribute("aria-describedby");
});

Note: useSlotId relies on a state update, so this test may need waitFor in JSDOM. If it's flaky, the story tests are already the authoritative coverage — just add a note explaining why the doc spec can't verify the attribute.

5. Interactive card docs show a semantic conflict [packages/nimbus/src/components/card/card.dev.mdx:151-173]

"Option 2" wraps Card.Root in a Box role="button". If that consumer also uses <Heading slot="title">, the card gets role="article" — an article nested inside a button is invalid ARIA (article is a landmark; landmark roles must not be descendants of interactive roles). The example should either:

• add a note: "Don't use slot-based labeling when the card is inside a button wrapper — use aria-label on the outer element instead", or
• replace the role="button" approach entirely with a real <button> (preferred).

Fix this →

Minor observations

6. Missing story for description-only slot case

TitleSlotOnly is tested. The a11y table in card.a11y.mdx documents Text slot="description" without a heading slot as a distinct supported case, but there's no story that verifies it. Not blocking, but the table makes a claim the test suite doesn't back.

7. alignItems: "flex-start" on root [packages/nimbus/src/components/card/card.recipe.ts:17]

Slot children are covered by their width: "100%". But free-form non-slot content placed directly in Card.Root will shrink-wrap instead of stretching. The WithoutCompound story renders a bare <div> inside the root — consumers might expect it to fill width. This is technically intentional given how the slots work, but it's worth a comment clarifying the behavior.

8. focusVisibleRing: "outside" in base styles [packages/nimbus/src/components/card/card.recipe.ts:19]

This style is unconditionally applied even to non-interactive cards. That's harmless (unfocusable elements never show the ring), but a brief comment would clarify this is in anticipation of tabIndex={0} usage rather than a mistake.

Architecture sign-off

• Direct rendering replacing context registration: correct and consistent with every other Nimbus compound component. ✓
• useSlotId + Provider pattern for ARIA wiring: idiomatic React Aria, tested end-to-end in stories. ✓
• --card-spacing CSS variable with adjacent sibling collapsing: clean, zero JS overhead. ✓
• variant / size instead of four ad-hoc props: correct normalization. ✓
• @react-aria/utils added as explicit dependency (was previously transitive): good hygiene. ✓
• display: flex vs inline-flex: correct for block-level containers. ✓
• App-level migrations (category-overview, frontpage): both correctly use Card.Body and compile cleanly. ✓